Updated BaseOutputHandler to accept state attributes - Tensorboard #2137
Conversation
There was a problem hiding this comment.
Thanks for the PR @Ishan-Kumar2 !
Looks good. I have a suggestion to remove TrainerStateHandler and put the new arg to OutputHandler.
There was a problem hiding this comment.
@Ishan-Kumar2 thanks for the update ! I liked the way you handled data type dispatch and I left a comment on how we could improve our loggers.
There was a problem hiding this comment.
Thanks for the update @Ishan-Kumar2
There few issues with docstrings otherwise, looks good.
| @@ -179,12 +179,9 @@ class OutputHandler(BaseOutputHandler): | |||
| Examples: | |||
|
|
|||
| .. code-block:: python | |||
|
|
|||
There was a problem hiding this comment.
I think these modifications are wrong, take a look how it is rendered now: https://deploy-preview-2137--pytorch-ignite-preview.netlify.app/generated/ignite.contrib.handlers.tensorboard_logger.html#ignite.contrib.handlers.tensorboard_logger.OutputHandler
Let's also update line 177:
- Helper handler to log engine's output and/or metrics
+ Helper handler to log engine's output, engine's state attributes and/or metricsThere was a problem hiding this comment.
This line 182 shouldn't be deleted. That's why the docstring is wrongly rendered.
|
@Ishan-Kumar2 I forgot to ask you to add ignite/ignite/handlers/stores.py Lines 38 to 39 in c6ff5b8 Please send a PR with a fix. By the way, would you like to update other loggers ? |
|
@Ishan-Kumar2 can you address this comment please for TB and Visdom. Yesterday I also forgot to ask about that. Thanks |
Addresses #2100 (using "fixes" will close the issue)
Description:
Adds feature to log state attributes in Tensorboard
Check list: